Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pre-commit checks and fix its complaints #547

Merged
merged 7 commits into from
Apr 3, 2024

Conversation

egeakman
Copy link
Member

Closes #497

We can change the maxkb argument to our needs.

Added some handy checks as well.

Since we are using pre-commit, we might want to add that to a requirements-dev.txt.

Copy link

vercel bot commented Mar 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ep2024 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 2, 2024 9:08pm

rev: v4.5.0
hooks:
- id: check-added-large-files
args: ["--maxkb", "5000"]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's currently set to 5MB

@patrick91
Copy link
Member

@egeakman this is amazing, can we add some instructions to the readme for installing pre-commit?

Also, maybe it is worth adding https://pre-commit.ci/ to this repo, so we are sure we always run it 😊

we should also probably add prettier!

@egeakman
Copy link
Member Author

I think it's a good idea to add pre-commit.ci here. I can't add it though, maybe a repository admin can?

@egeakman
Copy link
Member Author

Prettier's result is pretty noisy, it changes ~43 files. I'll push the changes, but we probably should check the preview carefully.

@patrick91
Copy link
Member

@artcz would you be ok with pre-commit ci? we could do a github action, but pre-commit ci is faster and free 😊

@egeakman
Copy link
Member Author

egeakman commented Mar 28, 2024

Prettier somehow breaks it.

It turns;

{/* into {/\*

and

*/} into \*/}

Oh, and it works in dev build, but not in prod build

@egeakman
Copy link
Member Author

egeakman commented Mar 28, 2024

Found lots of similar issues like this and this...

Turns out that prettier only supports MDX v1.

We can add an ignore flag to each problematic line, or we can simply skip all MDX files.

This is the final noise level, we can ignore some files such as pnpm-lock.yaml to reduce noise.

@egeakman
Copy link
Member Author

egeakman commented Apr 2, 2024

@patrick91 hi! I reduced the amount of changed/affected files by ~half.

prettier now:

  • doesn't touch the lock file,
  • doesn't add a trailing comma everywhere,
  • keeps the tab width consistently at 2 spaces

How should we proceed?

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

@patrick91 patrick91 merged commit 8ec69ff into EuroPython:main Apr 3, 2024
4 checks passed
@egeakman egeakman deleted the pre-commit branch April 3, 2024 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate if we can add precommit hook for commiting large files to the repository accidentally
2 participants